-
Notifications
You must be signed in to change notification settings - Fork 263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add AWS credential provider #825
base: master
Are you sure you want to change the base?
Add AWS credential provider #825
Conversation
Signed-off-by: Marcelo Bartsch <[email protected]>
e68a064
to
fd6270a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #825 +/- ##
==========================================
- Coverage 73.53% 73.20% -0.34%
==========================================
Files 31 31
Lines 3140 3157 +17
==========================================
+ Hits 2309 2311 +2
- Misses 695 710 +15
Partials 136 136 ☔ View full report in Codecov by Sentry. |
Thank you, I really do like this PR and the intent behind it. However, I do have slight concerns about maintainability here. To maintain this, one needs to have an aws account and ECR set up. How would this be integrated into a potential end-to-end test? Who will be the one to take care of issues and bugs reported to this particular functionality? Also, when we start incorporating proprietary authentication for vendor no. 1, then we're not far away from requests to incorporating similar mechanisms for vendor no. 2, vendor no. 3 and so on. This would multiply the impact described in the previous paragraph. All that being said, please don't close or abandon this PR. I just want to raise these concerns to open up the discussion. Thank you! |
About E2E test, they can be automated, of course you will need some infrastructure , or tools like localstack which emulate AWS, for other cloud platforms, no idea ,other option is to use mocked services,sadly I'm not that knowledged on testing in golang to implement , but is doable. Now in the case of new ones, you can just say 'Sure, provide a PR' , but I think is a natural path for people wanting to use this with less complications as possible, because, I could use the 'ext:/scripts/aws.sh' , for example, and just use that, but then again I need to modify the base image or mount it, but that might need aws cli, python and its depedencies. Again, just thinking out loud looking to expand the discussion! |
This code allows argo image updater to use ECR repos natively.
it is very basic code but get the job done, feel free to comment what is missing so I can update and improve this code.
thanks!